-
Notifications
You must be signed in to change notification settings - Fork 857
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
RFC: Add generic type support and improve java.util.{List,Map} handling #827
base: master
Are you sure you want to change the base?
Conversation
I'm glad that we're focusing on improving things in this area. We've had a few PRs lately that touch on List and Map handling for Java objects, and we made a change in the last release that resulted in it working differently than it did before. Would you, @tuchida, or someone else out there have the time to think about how we really want List and Map objects to show up in Rhino, based in part on what Nashorn did? I'm also fine with us having a few different choices based on a configuration parameter if we had to. I'm afraid to make any more changes in this area until we're all clear on where this should go. |
This looks good to me. This is just my preference, but I'd rather use Also, this isn't main topic, do you need a setter of var list = new java.util.ArrayList();
list.length; // 0
list.length = 3;
list; // [null, null, null]
list.length; // 3 |
Thanks for your feedback. First, a short introduction: A very early approach was, (before map/list support was added) that we converted our object graph to "NativeArray/NativeObjects". So the NativeJavaMap/List wrappers are really great. Some Must-Haves (from my perspective)
Some Good-To-Haves
Unclear:
public class Customer {
private String firstName;
private String lastName;
@ManyToMany
private List<Item> purchasedItems;
// ... getter and setters
} what should happen if you do a
|
For your purposes, does it make more sense for instead have a Java API that
lets a Java Map appear as a JavaScript "map"? That would eliminate all
ambiguity but require you to call some Rhino function to wrap a JS Map
around your Java Map.
…On Fri, Jan 22, 2021 at 4:52 AM Roland Praml ***@***.***> wrote:
Thanks for your feedback.
First, a short introduction:
We use rhino to access beans (data object) or a list/map of them (as you
do it in a lot of ORM applications)
A very early approach was, (before map/list support was added) that we
converted our object graph to "NativeArray/NativeObjects".
This was good, as long we did not modify anything. But changes on the
NativeArray/Object were not reflected back to the original object in the
object graph. (In addition there was much conversion overhead)
So the NativeJavaMap/List wrappers are really great.
For us, a java.util.Map/List should behave as much as possible as a
javscript Object/Array. This isn't always possible. E.g. the JS forEach(value,
key, map) is defined in java as forEach(key, value)
*Some Must-Haves (from my perspective)*
- Modifications on JS-objects must be reflected on the wrapped objects
(I think this point is indiscutable and also implemented)
- Types should be preserved: A bean that defines a property as Map<String,
Integer> should not store a double when you do a bean.myProp = 3.0 in
javaScript. This is what this PR addresses mainly
- for (elem in obj) and for each(elem in obj) should work the same way
as in javascript on java.util.Map/List (also addressed by this PR)
*Some Good-To-Haves*
- Support maps, where the key != String. (e.g. EnumMap) - it would be
ok for us, not to introcuce this change, as we can work around this, if we
really need.
- Support for each(elem in obj) for Iterables.
- list.length=3 should grow the list (Hint from @tuchida
<https://github.com/tuchida> )
*Unclear:*
- What should happen if I do this on a map javaMap.size=42. This
overwrites the size() method and I cannot get the size any more with
javaMap.size(). For this I made a fix, that existing functions cannot
be overwritten: ***@***.***
<FOCONIS@13483f1>
(Of course, I can use Object.keys(map).length)
- This is not mainly part of this PR, but as said above, we do a lot
of with beans:
public class Customer {
private String firstName;
private String lastName;
@manytomany
private List<Item> purchasedItems;
// ... getter and setters
}
what should happen if you do a customer.purchasedItems[] = {} and what
shoud happen if you do a Object.keys(customer)
- and last but not least JSON.stringify(customer) should work
—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
<#827 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAD7I26KI3CJAD4HTVSDJZTS3FYKNANCNFSM4WHA5NWQ>
.
|
Is this the test for rhino/testsrc/org/mozilla/javascript/tests/JavaListAccessTest.java Lines 24 to 28 in 6b4b1e2
Since these are separate features, it would have been easier to understand if it split commits or pull requests. [NITS] Do you need a |
[NITS] In the case of |
[NITS] var a = [];
a[10] = 123;
a[0]; // undefined
0 in a; // false This is probably the case for var list = new java.util.ArrayList();
list[10] = 123;
list[0]; // null
0 in list; // true Is this difference acceptable? |
An interesting idea, but I do not have control, where the objects are created. So I have a third-party class that returns a I took a lot of inputs and made a separate PR that extracts most of the list handling code. |
ref. #839 |
@gbrail I was thinking this would be preferable for me, if I could use a Rhino method to return a wrapped java.util.Map that always treated property access as getting or putting Map keys, and also still be able to have a reference to the original object where I could unambiguously access any Java method or field supported by the actual class implementing Map. I've noticed even Nashorn has some undesirable behavior dealing with this jjs> function hello(name) {print('hello ' + name)}
jjs> var map = new java.util.HashMap()
jjs> map.a = hello
function hello(name) {print('hello ' + name)}
jjs> map.get = hello
function hello(name) {print('hello ' + name)}
jjs> map.a('alice')
hello alice
jjs> map.get('bob')
null
jjs> map.get('get')('bob')
hello bob |
#820 (comment) |
# Conflicts: # src/org/mozilla/javascript/Context.java # src/org/mozilla/javascript/IdScriptableObject.java # src/org/mozilla/javascript/NativeIterator.java # src/org/mozilla/javascript/NativeJavaList.java # src/org/mozilla/javascript/NativeJavaMap.java # src/org/mozilla/javascript/NativeJavaObject.java # src/org/mozilla/javascript/ScriptRuntime.java # src/org/mozilla/javascript/WrapFactory.java # testsrc/org/mozilla/javascript/tests/NativeJavaMapTest.java
# Conflicts: # examples/PrimitiveWrapFactory.java # src/org/mozilla/javascript/NativeJavaMethod.java # src/org/mozilla/javascript/NativeJavaObject.java
# Conflicts: # examples/PrimitiveWrapFactory.java # src/org/mozilla/javascript/NativeJavaObject.java
# Conflicts: # src/org/mozilla/javascript/JavaMembers.java
What is the status of this PR? Is it still something that is planned to be merged at some point? If so, based on #972 (comment) I assume it still has breaking API changes and as such it should be added to the discussion list for v2.0.0? |
@p-bakker Yes, this would be a candidate for 2.0.0 - I plan to make this mergeable, but I need some time |
v2.0.0 is a long way of, so no rush. Just wanted to be sure whether I should include it in the v2.0.0 project |
# Conflicts: # src/org/mozilla/javascript/NativeJavaList.java # src/org/mozilla/javascript/NativeJavaMap.java
This PR is now ready for review. It adds generic type support, so that the generic type definitions in beans etc. are respected in
|
This PR is quite old, but still relevant for our fork, so I fixed the merge conflicts. |
We use rhino to access beans, so you can have a bean like this:
you can add the proper numbers.
Old behavoiur:
"bean.doubles[0] = 3" may store an integer object in
List<Double>
New behavoiur:
"bean.doubles[0] = 3" will be stored as
3.0D
As this change
WrapFactory.wrap(... Class staticType)
->WrapFactory.wrap(... Type staticType)
I would appreciate what other think of this change and if/how this change can be merged
I can split up this PR in more smaller PRs if the direction is clear